Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add support for TIME literal values #3010

Merged
merged 3 commits into from
Aug 15, 2022
Merged

feat: Add support for TIME literal values #3010

merged 3 commits into from
Aug 15, 2022

Conversation

stuartcarnie
Copy link
Contributor

@stuartcarnie stuartcarnie commented Aug 2, 2022 •
edited
Loading

This PR teaches DataFusion how to interpret TIME literals. These values are stored as nanoseconds from midnight using a Time64(Nanosecond).

Which issue does this PR close?

Closes #2883.

Rationale for this change

I adjusted the precision of TIME to nanosecond, which is different from the draft PR #2884 by @andygrove. The rationale for choosing Nanosecond is that PostgreSQL TIME requires microsecond precision. Microsecond precision requires the Time64(_) type, so it seems safe to increase the precision to Nanosecond with no additional memory requirements.

What changes are included in this PR?

Add support for TIME literals

Are there any user-facing changes?

This PR teaches DataFusion about TIME literals and therefore documentation would need to be revised.

@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions sql SQL Planner labels Aug 2, 2022
@stuartcarnie stuartcarnie mentioned this pull request Aug 2, 2022
Closed
@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2022 •
edited
Loading

Codecov Report

Merging #3010 (cc8caad) into master (48f9b7a) will decrease coverage by 0.00%.
The diff coverage is 78.04%.

@@            Coverage Diff             @@
##           master    #3010      +/-   ##
==========================================
- Coverage   85.95%   85.94%   -0.01%     
==========================================
  Files         291      291              
  Lines       52382    52420      +38     
==========================================
+ Hits        45025    45054      +29     
- Misses       7357     7366       +9
Impacted Files Coverage Δ
datafusion/common/src/scalar.rs 84.61% <63.15%> (-0.28%) ⬇️
datafusion/sql/src/planner.rs 81.99% <87.50%> (+0.04%) ⬆️
datafusion/core/tests/sql/timestamp.rs 99.78% <92.85%> (-0.22%) ⬇️
datafusion/expr/src/logical_plan/plan.rs 77.60% <0.00%> (-0.18%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@avantgardnerio
Copy link
Contributor

Thank you SO much for this! I struggled for a few days on this issue. Your help is very appreciated!

@waitingkuo waitingkuo mentioned this pull request Aug 10, 2022
Closed
@alamb alamb mentioned this pull request Aug 13, 2022
Closed
@github-actions github-actions bot removed physical-expr Physical Expressions logical-expr Logical plan and expressions labels Aug 15, 2022
stuartcarnie
stuartcarnie commented Aug 15, 2022
View reviewed changes
datafusion/sql/src/planner.rs
SQLDataType::Time => Ok(DataType::Time64(TimeUnit::Millisecond)),
SQLDataType::Time => Ok(DataType::Time64(TimeUnit::Nanosecond)),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change constitutes a fix as Time64(Millisecond) is invalid. Rather that switch to Time32(Millisecond), I moved to Time64(Nanosecond) as PostgreSQL requires at least microsecond precision per the documentation. Given Nanosecond comes at no additional cost, it seems reasonable to support the increase in precision.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is 👍

@stuartcarnie stuartcarnie marked this pull request as ready for review August 15, 2022 06:50
avantgardnerio
avantgardnerio approved these changes Aug 15, 2022
View reviewed changes
Copy link
Contributor

@avantgardnerio avantgardnerio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@waitingkuo
Copy link
Contributor

thank you @stuartcarnie !

note that time64 cannot be compared yet as there's no implementation in the kernel.

alamb
alamb approved these changes Aug 15, 2022
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great -- thanks @stuartcarnie and @avantgardnerio

datafusion/sql/src/planner.rs
SQLDataType::Time => Ok(DataType::Time64(TimeUnit::Millisecond)),
SQLDataType::Time => Ok(DataType::Time64(TimeUnit::Nanosecond)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is 👍

@alamb alamb merged commit 15a9a4b into apache:master Aug 15, 2022
@ursabot
Copy link

ursabot commented Aug 15, 2022

Benchmark runs are scheduled for baseline = 49a3b00 and contender = 15a9a4b. 15a9a4b is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@alamb alamb mentioned this pull request Aug 15, 2022
Merged
@stuartcarnie stuartcarnie deleted the sgc/issues/2883 branch August 15, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

@avantgardnerio avantgardnerio avantgardnerio approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate sql SQL Planner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for TIME literal values
6 participants
@stuartcarnie @codecov-commenter @avantgardnerio @waitingkuo @ursabot @alamb